Reduce ClusterState retention in retry closures by HarishNarasimhanK · Pull Request #20858 · opensearch-project/OpenSearch

HarishNarasimhanK · 2026-03-13T02:55:45Z

Description

1. Goal

In OpenSearch, snapshot deletion is a cluster manager routed operation. When a delete request is received, the cluster manager creates internal callback objects (listeners) to track the operation and notify the caller once it completes. These listeners inadvertently hold a reference to a large in-memory object called ClusterState, which contains the entire cluster's metadata, routing information, and index definitions.

When a snapshot deletion gets stuck or takes a long time to complete, users or automated systems may retry the delete request multiple times. As listeners accumulate from repeated retries, multiple ClusterState objects get pinned on the heap, causing the cluster manager node's memory usage to grow until it runs out of memory.

This change fixes the issue by ensuring that the listeners only hold the small pieces of information they actually need (a version number and a node identifier) instead of the entire ClusterState object. This allows the large ClusterState objects to be garbage collected immediately, preventing the memory buildup.

2. Current Workflow

This section traces the lifecycle of a snapshot delete request from the REST API to the point where the listener is stored in SnapshotsService.

A client sends DELETE /_snapshot/{repository}/{snapshot}.
The REST layer (RestDeleteSnapshotAction) constructs a DeleteSnapshotRequest and passes it to NodeClient.
NodeClient dispatches the request to TransportDeleteSnapshotAction, which extends TransportClusterManagerNodeAction.
The base class creates an AsyncSingleAction instance to manage the request lifecycle. AsyncSingleAction fetches the current ClusterState and calls doStart(clusterState).
If the local node is the cluster manager, doStart() wraps the original listener using getDelegateForLocalExecute(clusterState). This wrapper contains a lambda for retry logic that references the clusterState parameter. Due to Java lambda capture semantics, the entire ClusterState object is implicitly retained by this lambda for as long as the listener exists.
The wrapped listener is passed into TransportDeleteSnapshotAction.clusterManagerOperation(), which calls snapshotsService.deleteSnapshots(request, listener). The listener still carries the captured ClusterState reference inside its retry lambda.
Inside SnapshotsService, the deletion is submitted as a cluster state update. Once the cluster state is updated to record the deletion, the listener is stored in the snapshotDeletionListeners map (keyed by the deletion UUID) in order to notify the client when the deletion completes.

3. Issue with Current Workflow

The listener stored in snapshotDeletionListeners sits in the map until the repository-level deletion reaches a terminal state. If the deletion is stuck (due to slow I/O, stuck segment uploads, large repository cleanup, or any other reason), the listener remains in snapshotDeletionListeners indefinitely, and the captured ClusterState cannot be garbage collected.
For each subsequent delete request, SnapshotsService adds another listener to snapshotDeletionListeners through the same path. As delete requests accumulate, these listeners pile up, each pinning a ClusterState object on the heap. The cluster manager node's heap usage grows monotonically with each repeated delete, eventually leading to OutOfMemoryError.

4. Requirements

Reduce the size of the data retained by retry closures. Instead of capturing the full ClusterState object, closures should only hold the minimal primitives required for retry decisions.
Preserve existing retry behavior and backward compatibility.

5. Approach: Extract Primitives Before Closure Creation

The retry closures in TransportClusterManagerNodeAction only need two pieces of information from the ClusterState to make retry decisions: the cluster state version (a long) and the cluster manager node's ephemeral ID (a String). By extracting these values before creating any lambda or anonymous class, the closures capture only these lightweight primitives. The full ClusterState object is no longer referenced by any closure and becomes eligible for garbage collection immediately.

Sequence Diagram

Implementation Steps

In getDelegateForLocalExecute inside AsyncSingleAction, the cluster state version and the cluster manager node are extracted before the lambda is created. The lambda now references only these extracted values, so the full ClusterState is no longer retained.
A new overloaded retryOnMasterChange method is added that accepts the version and cluster manager node directly. It extracts the ephemeral ID from the cluster manager node and passes it to the predicate builder.
The original retryOnMasterChange method that accepts a full ClusterState is kept as a convenience bridge. It extracts the version and cluster manager node and delegates to the new overload.
The retry method signature is updated to accept the version and cluster manager node instead of the full ClusterState. It uses the persistent node ID and version to construct the cluster state observer via a new primitives-based constructor.
A new overloaded build method is added to ClusterManagerNodeChangePredicate that accepts the version and ephemeral ID directly. The existing build method that accepts a full ClusterState is refactored to extract these values and delegate to the new overload.
Two new constructors are added to ClusterStateObserver that accept the cluster manager node ID and version as primitives, instead of requiring a full ClusterState object.
The StoredState inner class inside ClusterStateObserver is refactored to support construction from primitives. The existing constructor that accepts a ClusterState now delegates to the new primitives-based constructor.

6. Validation

The fix was validated by reproducing the memory retention issue on a local cluster and comparing heap dumps before and after the change.

Reproduction Setup

Added a Thread.sleep() call in BlobStoreRepository.doDeleteShardSnapshots() to simulate a long-running deletion that stays stuck.
Created 500 indices with heavy mappings (50+ fields each), multiple aliases per index, and a small number of documents per index to inflate the ClusterState size.
Created one snapshot per index (500 snapshots total) using a filesystem-based snapshot repository.
Spammed delete requests for all snapshots repeatedly, so that listeners accumulate in snapshotDeletionListeners while the deletion is stuck.
Captured heap dumps from the cluster manager node and compared the retained size of listener chains.

Results

Before (without fix)

After (with fix)

Related Issues

Resolves #15065

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2026-03-13T02:57:35Z

PR Reviewer Guide 🔍

(Review updated until commit `b1a3f55`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes Sub-PR theme: Add primitive overload to ClusterManagerNodeChangePredicate Relevant files: server/src/main/java/org/opensearch/cluster/ClusterManagerNodeChangePredicate.java server/src/test/java/org/opensearch/cluster/ClusterManagerNodeChangePredicateTests.java Sub-PR theme: Add primitive constructor to ClusterStateObserver Relevant files: server/src/main/java/org/opensearch/cluster/ClusterStateObserver.java server/src/test/java/org/opensearch/cluster/ClusterStateObserverTests.java Sub-PR theme: Reduce ClusterState retention in retry closures Relevant files: server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java server/src/test/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeActionTests.java
⚡ Recommended focus areas for review ID Mismatch The new `ClusterStateObserver` constructor accepts `clusterManagerNodeId` as a persistent node ID (from `DiscoveryNode.getId()`), but the existing `StoredState` comparison logic uses `getClusterManagerNodeId()` which also returns the persistent ID. However, in `TransportClusterManagerNodeAction`, `retryOnMasterChange` extracts the ephemeral ID for the predicate but passes `clusterManagerNode` (not its ID) to `retry`, which then calls `clusterManagerNode.getId()` (persistent ID) for the observer. This is consistent, but it's worth verifying that the `StoredState` comparison in `ClusterStateObserver` uses the same ID type (persistent) as what's being stored, to ensure correct change detection. public ClusterStateObserver( String clusterManagerNodeId, long version, ClusterApplierService clusterApplierService, @Nullable TimeValue timeout, Logger logger, ThreadContext contextHolder ) { this.clusterApplierService = clusterApplierService; this.threadPool = clusterApplierService.threadPool(); this.lastObservedState = new AtomicReference<>(new StoredState(clusterManagerNodeId, version)); this.timeOutValue = timeout; if (timeOutValue != null) { this.startTimeMS = threadPool.relativeTimeInMillis(); } this.logger = logger; this.contextHolder = contextHolder; } Inconsistent ID Types In `retryOnMasterChange(long, DiscoveryNode, Throwable)`, the ephemeral ID is extracted for `ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)`, while in `retry(...)`, the persistent ID (`clusterManagerNode.getId()`) is used for the `ClusterStateObserver`. These two IDs serve different purposes and are used in different comparisons, but the asymmetry (ephemeral for predicate, persistent for observer) should be explicitly documented to avoid future confusion or bugs. private void retryOnMasterChange(long stateVersion, DiscoveryNode clusterManagerNode, Throwable failure) { final String ephemeralNodeId = clusterManagerNode != null ? clusterManagerNode.getEphemeralId() : null; retry(stateVersion, clusterManagerNode, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); } private void retry( final long stateVersion, final DiscoveryNode clusterManagerNode, final Throwable failure, final Predicate<ClusterState> statePredicate ) { if (observer == null) { final long remainingTimeoutMS = request.clusterManagerNodeTimeout().millis() - (threadPool.relativeTimeInMillis() - startTime); if (remainingTimeoutMS <= 0) { logger.debug(() -> new ParameterizedMessage("timed out before retrying [{}] after failure", actionName), failure); listener.onFailure(new ClusterManagerNotDiscoveredException(failure)); return; } final String persistentNodeId = clusterManagerNode != null ? clusterManagerNode.getId() : null; this.observer = new ClusterStateObserver( persistentNodeId, stateVersion, clusterService.getClusterApplierService(), Unused Test The test `testPrimitiveConstructorViaClusterService` creates a `ClusterService` mock and calls `clusterService.getClusterApplierService()`, but then directly constructs the observer with `clusterApplierService` rather than `clusterService`. The test name implies it tests the ClusterService path, but it actually only tests the ClusterApplierService constructor again. This may be misleading. public void testPrimitiveConstructorViaClusterService() { final ClusterApplierService clusterApplierService = mock(ClusterApplierService.class); final ClusterService clusterService = mock(ClusterService.class); when(clusterService.getClusterApplierService()).thenReturn(clusterApplierService); final ThreadPool threadPool = mock(ThreadPool.class); when(clusterApplierService.threadPool()).thenReturn(threadPool); when(threadPool.relativeTimeInMillis()).thenReturn(0L); final DiscoveryNode masterNode = new DiscoveryNode("master", buildNewFakeTransportAddress(), Version.CURRENT); final ClusterState newerState = ClusterState.builder(new ClusterName("test")) .nodes(DiscoveryNodes.builder().add(masterNode).clusterManagerNodeId(masterNode.getId())) .version(10) .build(); when(clusterApplierService.state()).thenReturn(newerState); // Use the ClusterApplierService-based constructor final ClusterStateObserver observer = new ClusterStateObserver( masterNode.getId(), 1L, clusterApplierService, TimeValue.timeValueSeconds(30), logger, new ThreadContext(Settings.EMPTY) ); final AtomicReference<ClusterState> receivedState = new AtomicReference<>(); observer.waitForNextChange(new ClusterStateObserver.Listener() { @Override public void onNewClusterState(ClusterState state) { receivedState.set(state); } @Override public void onClusterServiceClose() {} @Override public void onTimeout(TimeValue timeout) {} }); // Newer version detected — should accept immediately assertNotNull(receivedState.get()); assertEquals(10L, receivedState.get().version()); }

github-actions · 2026-03-13T02:58:11Z

PR Code Suggestions ✨

Latest suggestions up to b1a3f55

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Avoid retaining DiscoveryNode object in retry closure The `retry` method receives `clusterManagerNode` (a `DiscoveryNode`) but only uses it to extract `persistentNodeId` for the `ClusterStateObserver`. Passing the full `DiscoveryNode` object into the `retry` closure defeats the purpose of this PR, which is to avoid retaining the full `ClusterState` in closures. Consider passing only the persistent node ID (String) instead of the `DiscoveryNode` to `retry`. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [292-295] private void retryOnMasterChange(long stateVersion, DiscoveryNode clusterManagerNode, Throwable failure) { final String ephemeralNodeId = clusterManagerNode != null ? clusterManagerNode.getEphemeralId() : null; - retry(stateVersion, clusterManagerNode, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); + final String persistentNodeId = clusterManagerNode != null ? clusterManagerNode.getId() : null; + retry(stateVersion, persistentNodeId, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); } +private void retry( + final long stateVersion, + final String persistentNodeId, + final Throwable failure, + final Predicate<ClusterState> statePredicate +) { + if (observer == null) { + ... + this.observer = new ClusterStateObserver( + persistentNodeId, + stateVersion, + clusterService.getClusterApplierService(), + TimeValue.timeValueMillis(remainingTimeoutMS), + logger, + threadPool.getThreadContext() + ); + Suggestion importance[1-10]: 6 __ Why: The suggestion is valid — passing a full `DiscoveryNode` into the `retry` method partially defeats the memory optimization goal of this PR. However, the impact is limited since `DiscoveryNode` is much smaller than a full `ClusterState`, and the suggestion requires refactoring the `retry` method signature which adds complexity.	Low
General	Fix misleading test using unused mock object The test is named `testPrimitiveConstructorViaClusterService` and sets up a `ClusterService` mock, but then directly uses `clusterApplierService` in the constructor call instead of going through `clusterService.getClusterApplierService()`. The `ClusterService` mock is unused, making the test misleading. Either use `clusterService` in the constructor (if such a constructor exists) or rename the test to accurately reflect what is being tested. server/src/test/java/org/opensearch/cluster/ClusterStateObserverTests.java [261-284] -public void testPrimitiveConstructorViaClusterService() { +public void testPrimitiveConstructorViaClusterApplierService() { final ClusterApplierService clusterApplierService = mock(ClusterApplierService.class); - final ClusterService clusterService = mock(ClusterService.class); - when(clusterService.getClusterApplierService()).thenReturn(clusterApplierService); + final ThreadPool threadPool = mock(ThreadPool.class); + when(clusterApplierService.threadPool()).thenReturn(threadPool); + when(threadPool.relativeTimeInMillis()).thenReturn(0L); ... - // Use the ClusterApplierService-based constructor + // Use the ClusterApplierService-based constructor directly final ClusterStateObserver observer = new ClusterStateObserver( masterNode.getId(), 1L, clusterApplierService, - ... + TimeValue.timeValueSeconds(30), + logger, + new ThreadContext(Settings.EMPTY) ); Suggestion importance[1-10]: 4 __ Why: The observation is accurate — the `ClusterService` mock is set up but never used in the constructor call, making the test name `testPrimitiveConstructorViaClusterService` misleading. Renaming the test or removing the unused mock would improve clarity, but this is a minor test quality issue.	Low
Possible issue	Verify consistent node ID type usage in StoredState The new constructor uses `clusterManagerNodeId` as the persistent node ID for `StoredState`, but the existing `StoredState(ClusterState)` constructor uses `getClusterManagerNodeId()` which returns the persistent ID. However, the `isOlderOrDifferentMaster` comparison in `StoredState` compares `clusterManagerNodeId` fields — make sure the semantics are consistent. Looking at the `ClusterManagerNodeChangePredicate`, it uses the ephemeral ID for comparison, but `StoredState` uses the persistent ID. Verify that the new constructor is passing the correct type of node ID (persistent vs ephemeral) to maintain consistent behavior with the existing `StoredState(ClusterState)` constructor. server/src/main/java/org/opensearch/cluster/ClusterStateObserver.java [120-131] -public ClusterStateObserver( - String clusterManagerNodeId, - long version, - ClusterApplierService clusterApplierService, - @Nullable TimeValue timeout, - Logger logger, - ThreadContext contextHolder -) { - this.clusterApplierService = clusterApplierService; - this.threadPool = clusterApplierService.threadPool(); - this.lastObservedState = new AtomicReference<>(new StoredState(clusterManagerNodeId, version)); +// Ensure clusterManagerNodeId passed here is the persistent node ID (getId()), +// consistent with StoredState(ClusterState) which uses getClusterManagerNodeId() +this.lastObservedState = new AtomicReference<>(new StoredState(clusterManagerNodeId, version)); Suggestion importance[1-10]: 5 __ Why: This is a valid concern about consistency between persistent node IDs used in `StoredState` vs ephemeral IDs used in `ClusterManagerNodeChangePredicate`. However, looking at the PR code, the new constructor correctly passes the persistent node ID (via `clusterManagerNode.getId()`), consistent with `StoredState(ClusterState)` which uses `getClusterManagerNodeId()`. The `improved_code` only adds a comment without actual code change, making it more of a verification suggestion.	Low

Previous suggestions

Suggestions up to commit 424e410

Category	Suggestion	Impact
General	Avoid retaining DiscoveryNode in retry closure The `retry` method now accepts a `DiscoveryNode clusterManagerNode` parameter, but this parameter is only used to extract `persistentNodeId` for the `ClusterStateObserver` constructor. Passing the full `DiscoveryNode` object into the retry closure partially defeats the purpose of this PR (reducing `ClusterState` retention), since `DiscoveryNode` can itself hold references. Consider passing only the two primitive/string values (`stateVersion` and the persistent node ID string) to `retry` instead of the full `DiscoveryNode`. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [292-295] private void retryOnMasterChange(long stateVersion, DiscoveryNode clusterManagerNode, Throwable failure) { final String ephemeralNodeId = clusterManagerNode != null ? clusterManagerNode.getEphemeralId() : null; - retry(stateVersion, clusterManagerNode, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); + final String persistentNodeId = clusterManagerNode != null ? clusterManagerNode.getId() : null; + retry(stateVersion, persistentNodeId, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); } +private void retry( + final long stateVersion, + final String persistentNodeId, + final Throwable failure, + final Predicate<ClusterState> statePredicate +) { + if (observer == null) { + ... + this.observer = new ClusterStateObserver( + persistentNodeId, + stateVersion, + clusterService.getClusterApplierService(), + TimeValue.timeValueMillis(remainingTimeoutMS), + logger, + threadPool.getThreadContext() + ); + } + ... +} + Suggestion importance[1-10]: 6 __ Why: The suggestion is valid and consistent with the PR's goal of reducing object retention in closures. Passing only primitive/string values (`stateVersion` and `persistentNodeId`) instead of the full `DiscoveryNode` to `retry` would be a cleaner approach, though the impact is moderate since `DiscoveryNode` is much lighter than `ClusterState`.	Low
General	Avoid retaining DiscoveryNode in block-retry closure Similar to the `getDelegateForLocalExecute` refactoring, the `checkForBlock` method still passes a `DiscoveryNode` object into the `retry` closure. If `retry` is updated to accept only primitive/string values (as suggested above), this call site should also extract only the persistent node ID string to avoid retaining the `DiscoveryNode` reference in the lambda closure. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [474-476] final long blockStateVersion = localClusterState.version(); -final DiscoveryNode blockClusterManagerNode = localClusterState.nodes().getClusterManagerNode(); -retry(blockStateVersion, blockClusterManagerNode, blockException, newState -> { +final String blockPersistentNodeId = localClusterState.nodes().getClusterManagerNodeId(); +retry(blockStateVersion, blockPersistentNodeId, blockException, newState -> { Suggestion importance[1-10]: 5 __ Why: This is a follow-up to suggestion 1 and is contingent on refactoring `retry` to accept a `String` instead of `DiscoveryNode`. The suggestion is logically sound and consistent with the PR's memory-reduction goal, but depends on the first suggestion being applied first.	Low

Suggestions up to commit 586c22d

Category	Suggestion	Impact
General	Avoid retaining DiscoveryNode in retry closure The `retry` method now accepts a `DiscoveryNode clusterManagerNode` parameter, but this parameter is only used to extract `persistentNodeId` for the `ClusterStateObserver` constructor. Passing the full `DiscoveryNode` object into the retry closure partially defeats the purpose of this PR (reducing `ClusterState` retention), since `DiscoveryNode` can itself hold references. Consider passing only the two primitive/string values (`stateVersion` and `persistentNodeId`) directly to `retry` instead of the full `DiscoveryNode`. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [292-295] private void retryOnMasterChange(long stateVersion, DiscoveryNode clusterManagerNode, Throwable failure) { final String ephemeralNodeId = clusterManagerNode != null ? clusterManagerNode.getEphemeralId() : null; - retry(stateVersion, clusterManagerNode, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); + final String persistentNodeId = clusterManagerNode != null ? clusterManagerNode.getId() : null; + retry(stateVersion, persistentNodeId, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); } +private void retry( + final long stateVersion, + final String persistentNodeId, + final Throwable failure, + final Predicate<ClusterState> statePredicate +) { + if (observer == null) { + ... + this.observer = new ClusterStateObserver( + persistentNodeId, + stateVersion, + clusterService.getClusterApplierService(), + TimeValue.timeValueMillis(remainingTimeoutMS), + logger, + threadPool.getThreadContext() + ); + } + ... +} + Suggestion importance[1-10]: 6 __ Why: The suggestion is valid and consistent with the PR's goal of reducing object retention in closures. Passing only primitive/string values (`stateVersion` and `persistentNodeId`) instead of the full `DiscoveryNode` would be more aligned with the PR's intent, though `DiscoveryNode` is a relatively lightweight object compared to `ClusterState`.	Low
General	Avoid retaining DiscoveryNode in block retry closure Similar to the `getDelegateForLocalExecute` refactoring, the `DiscoveryNode` object `blockClusterManagerNode` is captured in the lambda closure. To fully avoid retaining unnecessary object references, only the persistent node ID string (used in the `ClusterStateObserver` constructor) should be captured in the closure, not the full `DiscoveryNode`. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [474-476] final long blockStateVersion = localClusterState.version(); -final DiscoveryNode blockClusterManagerNode = localClusterState.nodes().getClusterManagerNode(); -retry(blockStateVersion, blockClusterManagerNode, blockException, newState -> { +final String blockPersistentNodeId = localClusterState.nodes().getClusterManagerNodeId(); +final String blockEphemeralNodeId = localClusterState.nodes().getClusterManagerNode() != null + ? localClusterState.nodes().getClusterManagerNode().getEphemeralId() : null; +retry(blockStateVersion, blockPersistentNodeId, blockException, newState -> { Suggestion importance[1-10]: 5 __ Why: This suggestion is consistent with the PR's memory optimization goal and correctly identifies that `blockClusterManagerNode` (a `DiscoveryNode`) is captured in the lambda closure. However, it depends on suggestion 1's refactoring of the `retry` method signature, and the improvement is marginal since `DiscoveryNode` is much smaller than `ClusterState`.	Low

Suggestions up to commit d510a86

Category	Suggestion	Impact
General	Avoid retaining DiscoveryNode in retry closure The `retry` method now accepts a `DiscoveryNode clusterManagerNode` parameter, but this parameter is only used to extract `persistentNodeId` for the `ClusterStateObserver` constructor. Passing the full `DiscoveryNode` object into the `retry` closure partially defeats the purpose of this PR (reducing `ClusterState` retention), since `DiscoveryNode` objects can also hold references. Consider passing only the pre-extracted `persistentNodeId` (String) and `ephemeralNodeId` (String) as primitives to `retry` instead of the full `DiscoveryNode`. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [292-295] private void retryOnMasterChange(long stateVersion, DiscoveryNode clusterManagerNode, Throwable failure) { final String ephemeralNodeId = clusterManagerNode != null ? clusterManagerNode.getEphemeralId() : null; - retry(stateVersion, clusterManagerNode, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); + final String persistentNodeId = clusterManagerNode != null ? clusterManagerNode.getId() : null; + retry(stateVersion, persistentNodeId, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); } +private void retry( + final long stateVersion, + final String persistentNodeId, + final Throwable failure, + final Predicate<ClusterState> statePredicate +) { + if (observer == null) { + ... + this.observer = new ClusterStateObserver( + persistentNodeId, + stateVersion, + clusterService.getClusterApplierService(), + TimeValue.timeValueMillis(remainingTimeoutMS), + logger, + threadPool.getThreadContext() + ); + } + ... +} + Suggestion importance[1-10]: 5 __ Why: The suggestion has merit since passing a full `DiscoveryNode` into the `retry` closure partially defeats the PR's goal of reducing object retention. However, `DiscoveryNode` is a lightweight, immutable value object compared to `ClusterState`, so the improvement is marginal. The `improved_code` accurately reflects the proposed change.	Low

Suggestions up to commit 11428ae

Category	Suggestion	Impact
General	Avoid retaining DiscoveryNode in retry closure The `retry` method receives `clusterManagerNode` (a `DiscoveryNode`) but only uses it to extract `getId()` for the `ClusterStateObserver`. Passing the full `DiscoveryNode` object into the `retry` closure defeats the purpose of this PR, which is to avoid retaining the full `ClusterState` in closures. Consider passing only the persistent node ID string instead of the `DiscoveryNode` object. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [292-295] private void retryOnMasterChange(long stateVersion, DiscoveryNode clusterManagerNode, Throwable failure) { final String ephemeralNodeId = clusterManagerNode != null ? clusterManagerNode.getEphemeralId() : null; - retry(stateVersion, clusterManagerNode, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); + final String persistentNodeId = clusterManagerNode != null ? clusterManagerNode.getId() : null; + retry(stateVersion, persistentNodeId, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); } Suggestion importance[1-10]: 6 __ Why: The suggestion is valid - passing a full `DiscoveryNode` into `retry` when only `getId()` is used partially defeats the PR's goal of avoiding large object retention in closures. However, the impact is relatively minor since `DiscoveryNode` is much smaller than `ClusterState`, and this is a style/optimization concern rather than a correctness issue.	Low
	Replace DiscoveryNode parameter with String ID The `retry` method still accepts a `DiscoveryNode` parameter, which means the closure capturing this method's parameters still holds a reference to a `DiscoveryNode` object. Since only `clusterManagerNode.getId()` is used inside the method, the parameter type should be changed to `String persistentNodeId` to fully achieve the goal of reducing object retention in closures. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [297-314] private void retry( final long stateVersion, - final DiscoveryNode clusterManagerNode, + final String persistentNodeId, final Throwable failure, final Predicate<ClusterState> statePredicate ) { if (observer == null) { ... - final String persistentNodeId = clusterManagerNode != null ? clusterManagerNode.getId() : null; this.observer = new ClusterStateObserver( persistentNodeId, stateVersion, clusterService.getClusterApplierService(), Suggestion importance[1-10]: 6 __ Why: This suggestion correctly identifies that the `retry` method only uses `clusterManagerNode.getId()` and could accept a `String persistentNodeId` instead, which would be more consistent with the PR's goal. The improved code accurately reflects the change needed, though the impact is minor since `DiscoveryNode` is not as large as `ClusterState`.	Low
	Extract only node ID to avoid closure retention Similar to the `getDelegateForLocalExecute` refactoring, the `blockClusterManagerNode` `DiscoveryNode` object is captured in the lambda closure. Only the persistent node ID is needed by `retry`, so extracting just the ID string here would be consistent with the PR's goal and avoid unnecessary object retention. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [474-476] final long blockStateVersion = localClusterState.version(); -final DiscoveryNode blockClusterManagerNode = localClusterState.nodes().getClusterManagerNode(); -retry(blockStateVersion, blockClusterManagerNode, blockException, newState -> { +final String blockPersistentNodeId = localClusterState.nodes().getClusterManagerNodeId(); +retry(blockStateVersion, blockPersistentNodeId, blockException, newState -> { Suggestion importance[1-10]: 6 __ Why: This is a valid and consistent suggestion - extracting only the node ID string instead of the full `DiscoveryNode` aligns with the PR's stated goal of avoiding object retention in closures. However, this depends on suggestion 2 being applied first (changing `retry`'s signature), making it contingent on another change.	Low

Suggestions up to commit 2864a44

Category	Suggestion	Impact
Possible issue	Fix node ID type mismatch in retry predicate The `ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)` is called with the ephemeral ID, but the new `ClusterStateObserver` constructor stores the persistent ID (`clusterManagerNode.getId()`) in `StoredState`. The `StoredState.isOlderOrDifferentMaster` method compares `clusterManagerNodeId` against `clusterState.nodes().getClusterManagerNodeId()` which returns the persistent ID. This mismatch means the observer and predicate use different ID types, potentially causing incorrect change detection. Both should use the same ID type consistently. server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java [292-295] private void retryOnMasterChange(long stateVersion, DiscoveryNode clusterManagerNode, Throwable failure) { - final String ephemeralNodeId = clusterManagerNode != null ? clusterManagerNode.getEphemeralId() : null; - retry(stateVersion, clusterManagerNode, failure, ClusterManagerNodeChangePredicate.build(stateVersion, ephemeralNodeId)); + final String persistentNodeId = clusterManagerNode != null ? clusterManagerNode.getId() : null; + retry(stateVersion, clusterManagerNode, failure, ClusterManagerNodeChangePredicate.build(stateVersion, persistentNodeId)); } Suggestion importance[1-10]: 6 __ Why: This is a valid concern — `ClusterManagerNodeChangePredicate.build` uses ephemeral IDs while `StoredState` in `ClusterStateObserver` uses persistent IDs via `getClusterManagerNodeId()`. However, the original `ClusterManagerNodeChangePredicate.build(ClusterState)` also uses `getEphemeralId()`, so this may be intentional design. The inconsistency could cause subtle bugs with node restarts where ephemeral ID changes but persistent ID stays the same.	Low
Possible issue	Inconsistent node ID types between observer and predicate The new `ClusterStateObserver` constructor accepts a `clusterManagerNodeId` that is the persistent node ID (from `DiscoveryNode.getId()`), but `StoredState` compares it against `clusterState.nodes().getClusterManagerNodeId()` which also returns the persistent ID. However, in `retryOnMasterChange`, the `ClusterManagerNodeChangePredicate` is built using the ephemeral ID (`getEphemeralId()`), while `StoredState` uses the persistent ID. These two comparisons are inconsistent — the predicate uses ephemeral IDs but the observer uses persistent IDs. Ensure both use the same type of node identifier to avoid subtle bugs where a restarted node (same persistent ID, different ephemeral ID) is not detected as a change. server/src/main/java/org/opensearch/cluster/ClusterStateObserver.java [130] +// Either consistently use ephemeral IDs in both StoredState and ClusterManagerNodeChangePredicate, +// or consistently use persistent IDs in both. For example, if using persistent IDs: +// In retryOnMasterChange, pass clusterManagerNode.getId() instead of getEphemeralId() to ClusterManagerNodeChangePredicate.build() this.lastObservedState = new AtomicReference<>(new StoredState(clusterManagerNodeId, version)); Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about ID type consistency, but the `StoredState` uses persistent IDs while `ClusterManagerNodeChangePredicate` uses ephemeral IDs — these serve different purposes and are intentionally separate. The `improved_code` is essentially a comment, not an actual fix, making it less actionable.	Low

github-actions · 2026-03-13T03:01:32Z

Persistent review updated to latest commit 64a4b05

github-actions · 2026-03-13T04:19:11Z

❌ Gradle check result for 64a4b05: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-03-13T04:27:54Z

Persistent review updated to latest commit ee994e5

github-actions · 2026-03-13T04:46:24Z

❌ Gradle check result for ee994e5: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-03-13T04:51:08Z

Persistent review updated to latest commit 1ceb433

github-actions · 2026-03-13T06:17:19Z

✅ Gradle check result for 1ceb433: SUCCESS

codecov · 2026-03-13T06:18:22Z

Codecov Report

❌ Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.17%. Comparing base (04ac6e3) to head (b1a3f55).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...a/org/opensearch/cluster/ClusterStateObserver.java	93.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20858      +/-   ##
============================================
- Coverage     73.24%   73.17%   -0.07%     
+ Complexity    72811    72786      -25     
============================================
  Files          5871     5871              
  Lines        332666   332688      +22     
  Branches      48014    48017       +3     
============================================
- Hits         243660   243447     -213     
- Misses        69451    69776     +325     
+ Partials      19555    19465      -90

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions · 2026-03-13T09:18:10Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit b8784a1.

Path	Line	Severity	Description
server/src/test/java/org/opensearch/cluster/ClusterStateObserverTests.java	346	low	Trivially-passing assertion: `assertFalse("should not need to add listener", false)` always passes regardless of `listenerAdded.get()`, silently removing the verification that no timeout listener was registered. Should be `assertFalse("should not need to add listener", listenerAdded.get())`. Likely a typo but weakens test coverage for the new primitive constructor path.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-03-13T09:19:16Z

Persistent review updated to latest commit b8784a1

...ain/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java

github-actions · 2026-03-13T09:59:30Z

Persistent review updated to latest commit 9ed0978

github-actions · 2026-03-13T11:25:59Z

✅ Gradle check result for 9ed0978: SUCCESS

github-actions · 2026-03-13T11:46:58Z

Persistent review updated to latest commit ebe17a4

github-actions · 2026-03-13T13:14:10Z

❕ Gradle check result for ebe17a4: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

github-actions · 2026-03-13T19:14:22Z

Persistent review updated to latest commit 4195b48

github-actions · 2026-03-28T11:39:48Z

Failed to generate code suggestions for PR

github-actions · 2026-03-28T12:53:33Z

✅ Gradle check result for d4b3f62: SUCCESS

github-actions · 2026-03-30T04:16:25Z

Persistent review updated to latest commit 6a9755d

github-actions · 2026-03-30T05:23:58Z

❌ Gradle check result for 6a9755d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-03-30T07:50:27Z

Persistent review updated to latest commit 24c9147

github-actions · 2026-03-30T09:05:39Z

✅ Gradle check result for 24c9147: SUCCESS

github-actions · 2026-03-30T11:01:24Z

Persistent review updated to latest commit 2864a44

github-actions · 2026-03-30T11:37:10Z

❌ Gradle check result for 2864a44: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-03-30T11:49:47Z

Persistent review updated to latest commit 2864a44

github-actions · 2026-03-30T13:51:44Z

❌ Gradle check result for 2864a44: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-03-30T17:32:17Z

Failed to generate code suggestions for PR

github-actions · 2026-03-30T19:00:55Z

✅ Gradle check result for e68ebce: SUCCESS

github-actions · 2026-03-31T04:51:14Z

Persistent review updated to latest commit 11428ae

github-actions · 2026-03-31T05:48:17Z

Persistent review updated to latest commit d510a86

github-actions · 2026-03-31T07:00:10Z

❌ Gradle check result for d510a86: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-03-31T07:05:36Z

Persistent review updated to latest commit 586c22d

github-actions · 2026-03-31T08:22:35Z

❌ Gradle check result for 586c22d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-03-31T08:26:02Z

Persistent review updated to latest commit 424e410

github-actions · 2026-03-31T09:40:53Z

❌ Gradle check result for 424e410: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-03-31T09:42:30Z

Persistent review updated to latest commit b1a3f55

github-actions · 2026-03-31T11:10:38Z

✅ Gradle check result for b1a3f55: SUCCESS

HarishNarasimhanK requested a review from a team as a code owner March 13, 2026 02:55

github-actions bot added bug Something isn't working lucene Storage:Snapshots labels Mar 13, 2026

github-project-automation bot added this to Storage Project Board Mar 13, 2026

HarishNarasimhanK force-pushed the main branch from 1423309 to 64a4b05 Compare March 13, 2026 02:59

HarishNarasimhanK force-pushed the main branch from 64a4b05 to ee994e5 Compare March 13, 2026 04:26

HarishNarasimhanK force-pushed the main branch from ee994e5 to 1ceb433 Compare March 13, 2026 04:49

HarishNarasimhanK force-pushed the main branch from 1ceb433 to b8784a1 Compare March 13, 2026 09:17

Adiiigo reviewed Mar 13, 2026

View reviewed changes

...ain/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java Outdated Show resolved Hide resolved

Adiiigo reviewed Mar 13, 2026

View reviewed changes

...ain/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java Show resolved Hide resolved

HarishNarasimhanK force-pushed the main branch from b8784a1 to 9ed0978 Compare March 13, 2026 09:57

HarishNarasimhanK force-pushed the main branch from 9ed0978 to ebe17a4 Compare March 13, 2026 11:44

opensearch-ci-bot mentioned this pull request Mar 13, 2026

[AUTOCUT] Gradle Check Flaky Test Report for IndexServiceTests #14407

Open

Merge branch 'main' into main

d4b3f62

Merge branch 'main' into main

24c9147

HarishNarasimhanK added 2 commits March 30, 2026 17:30

Merge branch 'main' into main

0683fc3

Merge branch 'main' into main

e68ebce

linuxpi approved these changes Mar 31, 2026

View reviewed changes

Merge branch 'main' into main

b1a3f55

opensearch-ci-bot mentioned this pull request Mar 31, 2026

[AUTOCUT] Gradle Check Flaky Test Report for FullRollingRestartIT #18490

Open

opensearch-ci-bot mentioned this pull request Mar 31, 2026

[AUTOCUT] Gradle Check Flaky Test Report for IngestFromKafkaIT #17215

Open

Adiiigo approved these changes Mar 31, 2026

View reviewed changes

Conversation

HarishNarasimhanK commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

1. Goal

2. Current Workflow

3. Issue with Current Workflow

4. Requirements

5. Approach: Extract Primitives Before Closure Creation

Sequence Diagram

Implementation Steps

6. Validation

Reproduction Setup

Results

Related Issues

Check List

Uh oh!

github-actions bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit b1a3f55)

Uh oh!

github-actions bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions bot commented Mar 13, 2026

Uh oh!

github-actions bot commented Mar 13, 2026

Uh oh!

github-actions bot commented Mar 13, 2026

Uh oh!

github-actions bot commented Mar 13, 2026

Uh oh!

github-actions bot commented Mar 13, 2026

Uh oh!

github-actions bot commented Mar 13, 2026

Uh oh!

codecov bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Mar 13, 2026

PR Code Analyzer ❗

Uh oh!

github-actions bot commented Mar 13, 2026

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 13, 2026

Uh oh!

github-actions bot commented Mar 13, 2026

Uh oh!

github-actions bot commented Mar 13, 2026

Uh oh!

github-actions bot commented Mar 13, 2026

Uh oh!

github-actions bot commented Mar 13, 2026

Uh oh!

github-actions bot commented Mar 28, 2026

Uh oh!

github-actions bot commented Mar 28, 2026

Uh oh!

github-actions bot commented Mar 30, 2026

Uh oh!

github-actions bot commented Mar 30, 2026

Uh oh!

github-actions bot commented Mar 30, 2026

Uh oh!

github-actions bot commented Mar 30, 2026

Uh oh!

github-actions bot commented Mar 30, 2026

Uh oh!

github-actions bot commented Mar 30, 2026

Uh oh!

github-actions bot commented Mar 30, 2026

Uh oh!

github-actions bot commented Mar 30, 2026

Uh oh!

HarishNarasimhanK commented Mar 13, 2026 •

edited

Loading

github-actions bot commented Mar 13, 2026 •

edited

Loading

(Review updated until commit `b1a3f55`)

github-actions bot commented Mar 13, 2026 •

edited

Loading

codecov bot commented Mar 13, 2026 •

edited

Loading